Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make similarity client a stateful class #1432

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Make similarity client a stateful class #1432

wants to merge 2 commits into from

Conversation

alastair
Copy link
Member

Description
When I was looking at this similarity client for some gaia testing, I noticed that you can't have 2 clients, each pointing to a different host.

This change lets you pass in a host into the constructor.

There are still some changes to make - we need to read the settings file to get the host where this client is created.
I also changed to using requests, but we should improve the methods to pass GET args as a dict instead of constructing them manually.

@alastair alastair marked this pull request as ready for review May 25, 2020 08:41
@alastair
Copy link
Member Author

I've updated this to create an instance of the similarity client for both the normal server and the indexing server. This removes the requirement to have separate methods for add/save.

There are still some things that need to be improved:

  • requests 'timeout' by default is infinite. We should set this to a specific limit, and then undo it for save if needed
  • Still passing constructed parameters as the url, need to switch to params argument to requests.get. I'd like to be able to find a way to keep the tests the same (with a fully constructed url) in order to verify that we make this change correctly. I need to think about how these tests will fit together a bit better in order to do this
  • move client file from similarity/client/init.py to similarity/client.py
  • Fix conflicts

I still need to test with a real running similarity server

Copy link
Member

@ffont ffont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, and the missing changes you mention also look reasonable. About testing (once we pass query params as dict instead of building URL manually), I guess we just need to do assert_called_with and also check the params dict. Or if requests module has some method to "build URL" we could split the request in two parts, first build URL then call requests.get and we could mock requests.get. Doing a quick search I found you can do something like this:

requests.Request('GET', 'https://example.com/api', params=dict(param1='arg1',param2='arg2')).prepare().url 

Maybe this would work. Use this first then mock requests.get and do assert_called_with with the full URL.



similarity_client = Similarity(settings.SIMILARITY_ADDRESS)
indexing_similarity_client = Similarity(settings.INDEXING_SIMILARITY_ADDRESS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, love that

@alastair
Copy link
Member Author

yeah, I found the prepare().url syntax too. It's just a matter of seeing if I can write a test that contains the query parameter as "?foo=bar&etc", and then make the change to the code and see the tests stay the same. I'd prefer to use request's .get(baseurl, paramsdict) syntax, but maybe there's a middleground where I can construct the full url and pass it to .get() and then once the tests pass, move to the other syntax. I'll keep looking.

@alastair
Copy link
Member Author

Hi @ffont, I can't remember why we never finished this work. Was it really just because I couldn't agree on how to construct the test URLs? If that's it, then I propose that we quickly fix this and merge it. I'll try and resolve the conflicts.

@ffont
Copy link
Member

ffont commented Mar 17, 2023

Yeah, I guess that was it! Also similar thing should be done for tag recommendation, although I want to refactor it as a celery task and running on py3 so maybe there's no need to make this update now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants